Repository navigation
Conversation
yxshee
left a comment
There was a problem hiding this comment.
I ran this against current master (it merges cleanly): 12 tests fail, all with ReferenceError: os is not defined. isRunningLocally() uses os, but index.js never requires it, so any request with cookie.secure: true over plain HTTP throws. npm run lint also fails with a parsing error on the arrow function (index.js:717 after merging master), and Object.values and .flat() are not available on the Node 0.8 to 10 versions that CI still tests.
With var os = require('os') added, 6 existing tests still fail, for example "proxy option when enabled should work when no header: should not have Set-Cookie header". The reason is that os.networkInterfaces() returns the server's own interfaces, and loopback is always one of them. On macOS the check returns [ '127.0.0.1', '::1' ], and in a node:22-alpine container the addresses are 127.0.0.1 ::1 172.17.0.3. So isRunningLocally() returns true on effectively every host, and Secure cookies would be sent over plain HTTP in production too. That is the case the existing check is there to prevent.
What browsers allow for Secure cookies is decided by the address the client used to reach the server, not the server's interfaces. The closest per-request signal would be req.socket.localAddress or the Host header. That would still change a security default, though, so a maintainer may want to weigh in on whether this belongs in session at all.
From my understanding, if we are running our application locally without using HTTPS and Nginx or something similar, secure will not work under the logic of express-session.
In the latest version of Chrome and Firefox HTTPS requirements are ignored when the Secure attribute is set by localhost, please see the MDN documents below:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
Chrome permits setting the secure attribute for cookies under HTTP on localhost. However, express-session does not create cookies under HTTP, which is why cookies might not appear even if the secure and sameSite are correctly set.
I wonder if we can refine logic to align with Chrome rules, so I tried to do some 'naive' optimization by checking if application is running locally.
I am aware that there may be some security concerns related to my changes (Like if there are any other Reverse Proxys running locally under production mode, cookie will still be set even though they haven't set 'x-forwarded-proto' headers). If possible, could you happen to identify these issues so I can learn from them? Thank you! :)